http: revert 06cfff93, fixes overridden options by brendanashworth · Pull Request #1467 · nodejs/node

brendanashworth · 2015-04-18T19:45:50Z

This commit reverts commit 06cfff9 and adds a test that failed before
the revert. Options is in fact overridden in the code below the
util._extend() call (specifically options.port and options.host) to
allow for a one-argument net.createConnection(options) call.

Alternatives

Since this commit could be semver-major as it reverts functionality that's been there for a while, even though the parent commit introduced a regression that should've been semver-major too, we should look into alternatives. These are some I see:

say not all arguments passed to stdlib functions will be treated as immutable
for _http_client.js, handle createConnection differently (I already have different functionality we could try in a separate branch I haven't PR'ed yet). I'm 👍 on this functionality.

cc @jonathanong
ref #62 #593

bnoordhuis · 2015-04-18T19:56:39Z

test/parallel/test-http-dont-override-options.js

I thought const didn't work in sloppy mode?

It didn't originally error for me, but I also thought so too so I added 'use strict'; in the new commit.

bnoordhuis · 2015-04-18T19:58:29Z

Would it be possible to split this in git revert with an appropriate commit log (see e.g. 0526d83) and add the regression test in a follow-up commit? Having an 'official' revert commit log makes it easier to grep for them in the history.

This reverts commit 06cfff9. Reverted because it introduced a regression where (because options were modified in the later functionality) options.host and options.port would be overridden with values provided in other, supported ways.

This commit adds a test to ensure all options are NOT modified after passing them to http.request. Specifically options.host and options.port are the most prominent that would previously error, but add the other options that have default values. options.host and options.port were overridden for the one-argument net.createConnection(options) call.

brendanashworth · 2015-04-18T20:37:03Z

Updated for your comments @bnoordhuis. The revert commit has a title of longer than 50 chars now, I hope that is ok?

bnoordhuis · 2015-04-18T21:12:12Z

Yes, revert commits get dispensation. LGTM.

brendanashworth · 2015-04-18T21:14:27Z

Is this okay as semver-patch?

bnoordhuis · 2015-04-18T21:17:26Z

I would say so. It fixes a regression.

brendanashworth · 2015-04-18T21:18:40Z

Awesome, merging.

This reverts commit 06cfff9. Reverted because it introduced a regression where (because options were modified in the later functionality) options.host and options.port would be overridden with values provided in other, supported ways. PR-URL: #1467 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit adds a test to ensure all options are NOT modified after passing them to http.request. Specifically options.host and options.port are the most prominent that would previously error, but add the other options that have default values. options.host and options.port were overridden for the one-argument net.createConnection(options) call. PR-URL: #1467 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

brendanashworth · 2015-04-18T21:47:02Z

Thanks, this was merged in 7180597 and 6bf85bc.

jonathanong · 2015-04-19T21:05:10Z

¯\_(ツ)_/¯

jonathanong · 2015-04-19T21:05:55Z

personally i would at least change it to options = Object.create(options || {}) but i think other people care about inheritance

Fishrock123 · 2015-04-19T21:48:17Z

.. isn't this a breaking change though?

I guess it landed on master, but still. Weekend patches.

rlidwka · 2015-04-19T21:55:53Z

personally i would at least change it to options = Object.create(options || {})

+1

brendanashworth · 2015-04-19T21:58:27Z

@jonathanong maybe reopen #592?

jonathanong · 2015-04-19T22:03:53Z

sure

jonathanong · 2015-04-19T22:21:37Z

nope... too many tests are failing for me.

brendanashworth added the http Issues or PRs related to the http subsystem. label Apr 18, 2015

bnoordhuis reviewed Apr 18, 2015
View reviewed changes

brendanashworth added 4 commits April 18, 2015 13:22

!fixup fix test's localhost for FreeBSD runs

ff88a4f

!fixup more verbose test name

6e5cc0a

brendanashworth force-pushed the http-dont-copy-options branch from 1c7778d to 6e5cc0a Compare April 18, 2015 20:35

brendanashworth self-assigned this Apr 18, 2015

brendanashworth closed this Apr 18, 2015

brendanashworth removed their assignment Apr 18, 2015

rvagg mentioned this pull request Apr 27, 2015

Release proposal: v2.0.0 #1532

Closed

brendanashworth deleted the http-dont-copy-options branch May 5, 2015 03:09

brendanashworth mentioned this pull request Jul 23, 2015

https: reuse TLS sessions in Agent #2228

Closed

Uh oh!

Conversation

brendanashworth commented Apr 18, 2015

Alternatives

Uh oh!

bnoordhuis Apr 18, 2015

Choose a reason for hiding this comment

Uh oh!

brendanashworth Apr 18, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Apr 18, 2015

Uh oh!

brendanashworth commented Apr 18, 2015

Uh oh!

bnoordhuis commented Apr 18, 2015

Uh oh!

brendanashworth commented Apr 18, 2015

Uh oh!

bnoordhuis commented Apr 18, 2015

Uh oh!

brendanashworth commented Apr 18, 2015

Uh oh!

brendanashworth commented Apr 18, 2015

Uh oh!

jonathanong commented Apr 19, 2015

Uh oh!

jonathanong commented Apr 19, 2015

Uh oh!

Fishrock123 commented Apr 19, 2015

Uh oh!

rlidwka commented Apr 19, 2015

Uh oh!

brendanashworth commented Apr 19, 2015

Uh oh!

jonathanong commented Apr 19, 2015

Uh oh!

jonathanong commented Apr 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants